Skip to content

Add mgx mtx function#28

Merged
sterrettJD merged 38 commits intomainfrom
add_mgx_mtx_function
Apr 24, 2025
Merged

Add mgx mtx function#28
sterrettJD merged 38 commits intomainfrom
add_mgx_mtx_function

Conversation

@sterrettJD
Copy link
Owner

No description provided.

klterwelp added 29 commits April 9, 2025 17:09
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specify in docstrings that feature.table is mtx

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed 478318a

@sterrettJD sterrettJD requested a review from klterwelp April 16, 2025 20:28
R/mtxDE.R Outdated
#' column added to them
#'
#' @keywords internal
.add_dna_to_formula <- function(data, col, formula, fixed.vars, reg.method) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.vars=NULL

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in 478318a

@sterrettJD
Copy link
Owner Author

Hey Kat, I think the main thing still needed is the last integration tests for mtxDE. I can add these if you'd like, but let me know if you'd like to do that instead.

What I mean by integration tests is just testing where a full example of mtxDE is run with a dna table too. I think all of the new intermediates have been tested, which is great!

@klterwelp
Copy link
Collaborator

An integration test has been on my to-do list! Since I wrote the code, is it better for me to do it? Or, is it better to have you test my code as an "outsider" point of view?

Which do you prefer? It'll be next on my to do list after simulations :)

@sterrettJD
Copy link
Owner Author

Hey @klterwelp, I went ahead and added a few basic integration tests. The lm tests pass, but the gamlss tests unfortunately fail due to some weird behavior with the GAMLSS models, where either the phenotype or MGX feature won't exist in the report.

That's because the parameter estimates are NA when broom.mixed::tidy goes to summarize them. I'm not quite sure why they're NA, but maybe bigger sample sizes would fix this? Or it's something about the distributions of the data in these tests?

Have you run into this? I can do some more digging soon.

@klterwelp
Copy link
Collaborator

That's weird! The gamlss method does give me estimates for "_mgx" and phenotype using the mtx2021 simulations. Maybe try running it with one of those to see if the behavior persists?

@sterrettJD
Copy link
Owner Author

Hey @klterwelp I found that it's mostly sample-size dependent. Ran some other examples where very small samples cause these to be NA, whereas larger samples don't have that issue. I just made more realistic dataset. It may be worth adding a check for this in the output, but I think it's okay for now

@sterrettJD sterrettJD merged commit 0bc869e into main Apr 24, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants